Seems like some code that was forgotten

Thanks for checking out this PR @agoscinski . For some background info: I commented this out on purpose for now. Ideally, this check should be there because when storing the code, this condition should be satisfied. Unfortunately, however, this now also prevents just instantiating this model that validates the check, even if we don't plan on storing it. This is problematic for the export behavior, because that would rely on instantiating a Model that violates this uniqueness rules, even though in this case it doesn't apply.

The old CLI verdi code create core.code.installed would also rely on something like this check to immediately return an error as soon as label was specified in interactive mode and that label already existed for the same computer. It would then reprompt the user asking for a different label. I haven't been able to reimplement this feature in this PR so was keeping the comment as a reminder.

Safe to say that this PR is not completely done yet 😅

Is this still "in progress"?

@sphuber I'm looking at this now. Trying to probe the issue you mention in this thread. I did the following:

1. Create a hello_world@localhost installed code using verdi
2. Load the code and serialize it
3. Create an InstalledCode instance from 2
   I expected an issue here, as from_serialized leverages the model, so the validator should've been triggered, raising an exception, as the label + computer already exist! No exception though.

As for your comment regarding the CLI, are you saying a similar validation should be implemented there? Sounds like it, but not in this PR, right?

Summarizing a recent meeting with @sphuber:

• The currently commented out validator on the Code.Model is intended as a check on the uniqueness of Code label+computer.label tuples
• However, one use of the pydantic Model of a code, the dynamic creation of the interactive options of verdi code create..., requires the Model only for access to the available properties. Unfortunately, the validator will fire on instantiation from the serialized code, which will conflict with the uniqueness constraint, making it impossible to use the Model in this case
• The move to pydantic Models for this CLI application regresses the user experience in the sense that it does not fire the validator until after the user finishes the interactive part, even though the tuple is already available within the first three options (note that this is not tied to this PR, as the use of pydantic Models for this case was implemented in an earlier PR)

The CLI matter is not blocking for this PR, as I understand it. A solution can be explored in a follow-up PR.

@sphuber one thing that is still unclear to us is how in the CLI application a Model is created from an existing Code. Are we not discussing the creation of codes? How is there already a code?

It doesn't create a Model from an existing code. What the CLI code does is directly construct a Model from the values passed by the user to the CLI command. And then uses InstalledCode.from_model(model) to instantiate the InstalledCode instance which it then will store. The relevant line in the code:
https://github.com/sphuber/aiida-core/blob/d4c4d94851836e58e743d8f1a2d30c665af2ae3b/src/aiida/cmdline/commands/cmd_code.py#L35

So in that particular use case, the CLI implementation for creating codes, the fact that instantiating InstalledCode.Model for a label and computer that already exist raises a validation error is actually exactly what we want. Because we intend to then store an actual InstalledCode instance from that model directly after. The problem only comes in that now we can never instantiate this Model even if we never intend to store it as another ORM instance. The main example is when we want to serialize an existing ORM instance through the Model as an intermediate, as we have discussed in the comments above.

Thanks @sphuber. Two things:

1. Regarding unintended effects of the serialize_computer guard, I would argue that any situation prior to the future merging of this PR that required the label of the computer was a situation in which the computer was indeed an instance of Computer and thus would not be affected by this change. The only cases where computer is a str should be the serialization case, no?
2. Regarding the CLI case...

The problem only comes in that now we can never instantiate this Model even if we never intend to store it as another ORM instance. The main example is when we want to serialize an existing ORM instance through the Model as an intermediate, as we have discussed in the comments above.
Are you saying there IS a problem still, or are you simply reiterating the original problem? Because it seems to me that the use of model_construct is precisely the solution. Or am I missing something?

Regarding unintended effects of the serialize_computer guard, I would argue that any situation prior to the future merging of this PR that required the label of the computer was a situation in which the computer was indeed an instance of Computer and thus would not be affected by this change. The only cases where computer is a str should be the serialization case, no?

I am not worried about existing code, since the serialization is completely new. But I was just saying that with adding this workaround, although it fixes the immediate problem with using model_construct (which is itself already a bit of a workaround), I cannot guarantee that this will not lead to other problems/inconsistencies in the future. But perhaps this is still the best solution for now.

Are you saying there IS a problem still, or are you simply reiterating the original problem? Because it seems to me that the use of model_construct is precisely the solution. Or am I missing something?

No I was simply answering your question and providing more explanation. Using model_construct in the serialization code does seem to workaround the problem for now (in combination with the guard in the computer serializer)

Okay. Thanks for clarifying this matter 🙏

Hmm, I think maybe there's another potentially critical issue here.

I tried reconstructing an installed code instance. With the validation active on the model, I first need to make the above change (and related changes) to Entity.to_model(). This allows me to serialize the code. However, deserializing it using orm.InstalledCode.from_serialized(**code.serialize()) also raises the validation error. But here we shouldn't assume pre-validated input, so we can't use the model_construct solution in from_serialized.

I'm not sure how to get around this matter and allow the model validator. A REST API receiving a a serialized Code instance cannot assume the data is valid. In the case that the data came from the same AiiDA instance with which the REST API is linked, the validation will surely object.

@sphuber am I overlooking something?

update

Thinking on this some more, "real" use cases of REST API node transfer will likely be between AiiDA instances, so the collision of the same full code label should not happen? But even if this is true, a specialized warning would be better. Not sure where to implement it though.

Is it really necessary to make orm pluggable, what is the use case of it?

@sphuber correct me if I'm wrong, but this has to do with the use of orm_class, right? Under the implementation section of the AEP, it states that orm_class is a shortcut to model_to_orm used to define a model field that points to an ORM instance. So instead of

class Model(...):
    user = MetadataField(
        ...,
        model_to_orm=lambda pk: User.collection.get(id=pk),
    )

you can do

class Model(...):
    user = MetadataField(
        ...,
        orm_class=User,
    )

The various ORM classes that may be used in orm_class are then defined in project.entry-points.'aiida.orm', so that you can do in Entity.model_to_orm_field_values(...):

if orm_class := get_metadata(field, 'orm_class'):
    if isinstance(orm_class, str):
        orm_class = BaseFactory('aiida.orm', orm_class)
        fields[key] = orm_class.collection.get(id=field_value)

But then I guess my question is, do these not already have entry points? Or is the problem that you can't fetch them all generically using the BaseFactory?

Why?

This is related to the changes of adding the aiida.orm entry point group and making it possible to load the ORM classes through an entry point. But I will have to go back to the PR and actually test to see if this is still necessary

Why?

@sphuber do you remember why you added this CHECK_FILENAME class attribute?